-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PatternLang] Don't rewrite expressions used outside of the pattern #5930
[PatternLang] Don't rewrite expressions used outside of the pattern #5930
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes LGTM. Leave some comments to make sure I understand the semantic correctly.
T2 = BN[0] | ||
add = T1 + T2 | ||
|
||
assert tuple_get_item_node.partition(add) == add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two questions to this test case:
- Should we use
structural_equal
here? - Does that mean we do not even treat
BN -> T2
as a match anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partitioning either path is invalid, since either side would need intermediate nodes from the other, so we expect the original expression to come back unchanged, thus the ==
. We treat it as a match, but we don't treat it as something we can independently rewrite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. That makes sense.
relu = relay.op.nn.relu(conv2d) | ||
out = relu + conv2d | ||
|
||
assert pattern.partition(out) == out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, fusing the conv and relu would make the rest of the expr invalid, so we expect the expr to come back unchanged.
Thanks @mbrookhart , Thanks @comaniac for reviewing |
…pache#5930) * Don't rewrite expressions used outside of the pattern * add comments
…pache#5930) * Don't rewrite expressions used outside of the pattern * add comments
@comaniac
While investigating #5928, I discovered an issue where the pattern language would rewrite expressions used outside of the pattern, this prevents that issue, but doesn't fix #5928